CSE Machine : Fix Highlighting#3690
Conversation
github-project-automation
Bot
moved this to Todo
in SA 2627 CP3108 P05: CSE Machine liveness
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the user experience of the CSE Machine visualization by enhancing the arrow highlighting mechanism. Instead of only highlighting the arrow itself, the system now also highlights the immediate source element of the arrow upon hover or click. This change provides more intuitive visual feedback, helping users better understand the connections and flow within the diagram without introducing the complexity of recursive path highlighting. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a nice enhancement to the CSE machine visualization, highlighting the source of an arrow when it's hovered or clicked. The implementation is spread across multiple components, correctly adding optional methods to the IVisible interface and implementing them where needed. The change to allow toggling arrow selection on click is also a good usability improvement.
My review has identified several instances of code duplication in the new style handling methods. While the logic is correct, extracting the duplicated code into helper methods would improve maintainability. I've left specific comments with suggestions on how to refactor this.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a useful feature for highlighting the source of an arrow upon hover or click, enhancing the CSE Machine's interactivity. The implementation is well-structured across various components. My review focuses on a couple of areas where code quality and maintainability can be improved by refactoring to reduce duplication and enhance property access patterns.
Akshay-2007-1
moved this from Todo
to In Progress
in SA 2627 CP3108 P05: CSE Machine liveness
There was a problem hiding this comment.
Pull request overview
Enhances CSE Machine arrow interactions so that hovering/clicking an arrow also highlights the arrow’s immediate source (e.g., frame/value/control/stash item), improving diagram readability for students.
Changes:
- Add source-highlight hooks (
setArrowSourceHighlightedStyle/setArrowSourceNormalStyle) across key CSE diagram nodes (frames, values, stack items, etc.). - Update
GenericArrowto apply/revert source highlighting on hover/selection, including click-to-toggle selection behavior. - Introduce shared styling helpers in
Visibleto support applying highlight/normal styles to component shapes.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/features/cseMachine/components/values/GlobalFnValue.tsx | Adds arrow-source highlight/normal styling for global function values. |
| src/features/cseMachine/components/values/FnValue.tsx | Adds arrow-source highlight/normal styling for non-global function values. |
| src/features/cseMachine/components/values/ContValue.tsx | Adds arrow-source highlight/normal styling for continuation values. |
| src/features/cseMachine/components/arrows/GenericArrow.tsx | Invokes source highlight/normal styling and supports deselect-to-normal on click. |
| src/features/cseMachine/components/Visible.tsx | Adds shared setShapesStyle helper and base no-op arrow-source style hooks. |
| src/features/cseMachine/components/Text.tsx | Implements arrow-source highlight/normal styling for text nodes. |
| src/features/cseMachine/components/StashItemComponent.tsx | Implements arrow-source highlight/normal styling for stash items. |
| src/features/cseMachine/components/Frame.tsx | Implements arrow-source highlight/normal styling for frames (rect + title). |
| src/features/cseMachine/components/ControlItemComponent.tsx | Implements arrow-source highlight/normal styling for control items. |
| src/features/cseMachine/components/ArrayUnit.tsx | Implements arrow-source highlight/normal styling for array units. |
| src/features/cseMachine/CseMachineTypes.ts | Extends IVisible with optional arrow-source highlight/normal hooks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
…tion of live vs dead while highlighting as well!
|
In the latest commit, I have included all the suggestions @martin-henz has given us here 
As in the image, it features a seperate color for objects and arrows that are highlighted due to being selected or hovered on based on whether they are live or dead. Another thing to add is that, now the target of the arrow along with the source is highlighted as well! Another nuance that should be observed is that if an arrow and its source are dead, it is not necessary that the target is dead. In those cases, when hovered on, the arrow and the source will be highlighted in a duller tone as compared to the target which is very much alive haha! |
|
/gemini review The code segment has already been reviewed multiple times and this is potentially the last time due to the newer additions |
There was a problem hiding this comment.
Code Review
This pull request introduces new styling capabilities for the CSE machine visualization, allowing elements to display different highlighted and normal styles based on their 'live' or 'dead' state. This is achieved by adding new color configurations, extending the IVisible interface with setArrowSourceHighlightedStyle and setArrowSourceNormalStyle methods, and implementing these methods across various UI components like ArrayUnit, Frame, Text, and different Value types. The GenericArrow component now utilizes these new methods to update the styles of its source and target elements. Feedback suggests improving type safety in Visible.tsx by explicitly typing Konva shapes and addressing a potentially redundant else block in GenericArrow.tsx's onClick method.
| this.arrowHeadRef.current.pointerWidth(Config.ArrowHeadSize); | ||
| this.arrowHeadRef.current.pointerLength(Config.ArrowHeadSize); | ||
| } | ||
| this.source.setArrowSourceNormalStyle?.(); | ||
| this.target?.setArrowSourceNormalStyle?.(); |
There was a problem hiding this comment.
Bug: Hovering away from an unselected arrow can incorrectly remove the highlight from a shared node that is part of a separate, selected arrow.
Severity: CRITICAL
Suggested Fix
Update the setNormalStyle() and setArrowSourceNormalStyle() methods. Before resetting a node's style, they should check the global selection state to verify if the node is part of any currently selected arrow. If it is, the highlight style should be maintained instead of being reset.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/features/cseMachine/components/arrows/GenericArrow.tsx#L203-L207
Potential issue: When an arrow is selected, its source and target nodes are highlighted.
If the user then hovers over and away from a different, unselected arrow that shares a
source or target node, the shared node's highlight is incorrectly removed. This occurs
because the `onMouseLeave` event on the unselected arrow triggers `setNormalStyle()`,
which unconditionally resets the node's style via `setArrowSourceNormalStyle?.()`. This
reset logic is unaware that the node should remain highlighted because it is part of
another arrow that is still selected, leading to incorrect visual feedback for the user.
There was a problem hiding this comment.
doc_2026-03-26_14-15-48.mp4
This is a real issue!
@gemini-code-assist how can we fix this simply
There was a problem hiding this comment.
You are absolutely right, this is a critical issue that needs to be addressed. The bug occurs because setArrowSourceNormalStyle() is called unconditionally on onMouseLeave for an unselected arrow, which can inadvertently remove the highlight from a shared node that is still part of a selected arrow. This leads to incorrect visual feedback.
To fix this simply, we need to ensure that a node's style is only reset if it is not currently highlighted by the globally selected arrow. This requires a two-part change:
-
Add a helper method to
ArrowSelectionManager: This method will check if a givenIVisiblenode is the source or target of the currently selected arrow. You can add this method tosrc/features/cseMachine/components/arrows/ArrowSelection.ts:// In src/features/cseMachine/components/arrows/ArrowSelection.ts // Inside the ArrowSelectionManager class isNodeHighlightedBySelectedArrow(node: IVisible): boolean { if (!this.selectedArrow) { return false; } return this.selectedArrow.source === node || this.selectedArrow.target === node; }
-
Modify
GenericArrow.setNormalStyle(): Update thesetNormalStylemethod insrc/features/cseMachine/components/arrows/GenericArrow.tsxto use this new helper method. This will prevent the source or target node's style from being reset if it's part of the currently selected arrow.
I've provided a code suggestion below for the change in GenericArrow.tsx that directly addresses the lines in this comment thread.
| this.arrowHeadRef.current.pointerWidth(Config.ArrowHeadSize); | |
| this.arrowHeadRef.current.pointerLength(Config.ArrowHeadSize); | |
| } | |
| this.source.setArrowSourceNormalStyle?.(); | |
| this.target?.setArrowSourceNormalStyle?.(); | |
| this.arrowHeadRef.current.pointerWidth(Config.ArrowHeadSize); | |
| this.arrowHeadRef.current.pointerLength(Config.ArrowHeadSize); | |
| } | |
| // Only reset source/target style if they are not part of the currently selected arrow | |
| if (this.source && !arrowSelection.isNodeHighlightedBySelectedArrow(this.source)) { | |
| this.source.setArrowSourceNormalStyle?.(); | |
| } | |
| if (this.target && !arrowSelection.isNodeHighlightedBySelectedArrow(this.target)) { | |
| this.target.setArrowSourceNormalStyle?.(); | |
| } |
There was a problem hiding this comment.
@martin-henz @RichDom2185 @Oofky
I would appreciate your help and suggestions as to if this is necessary or we can ignore it since its a minor feature?
There was a problem hiding this comment.
yes, feel free to ignore. You may want to add a comment why.
There was a problem hiding this comment.
Yes, can ignore from this PR. But please create an issue that references this comment thread.
github-project-automation
Bot
moved this from In Progress
to Done
in SA 2627 CP3108 P05: CSE Machine liveness
Description
Appends an additional minor functionality where on hover or on click, instead of just highlighting the arrow, we also highlight its immediate source. Other possible variations where we highlight the entire recursive backtracked path from the Global frame has been discussed but discarded in the comments of #3683 because it may cause confusion for the users.
An additional point being that it also highlights a dead arrow on clicking since its a conscious choice by the user.
Closes #3683.
Type of change
How to test
Any code on running with a valid CSE Machine diagram can be used to hover over arrows or clicked upon. Some excerpts through pictures are shown below :
Checklist